Skip to content

Add v128 type and SIMD intrinsics#2

Open
luchak wants to merge 11 commits intoexoticorn:masterfrom
luchak:simd
Open

Add v128 type and SIMD intrinsics#2
luchak wants to merge 11 commits intoexoticorn:masterfrom
luchak:simd

Conversation

@luchak
Copy link
Copy Markdown

@luchak luchak commented Jan 24, 2023

I present: a silky-smooth yak. I've tried to add support for the v128 type and WASM SIMD intrinsics.

Major warning: I have never written Rust before.

The biggest challenge I encountered was supporting instructions that take lane parameters. Following the way load and store instructions are treated now, I've added special handling for two new kinds of instructions: MemLaneInstruction (loads and stores) and LaneInstruction (extract and replace lane). I'm not happy with my naming or the very copy/paste way that these new formats are handled. I'm sure something much better could happen here but I'm not comfortable enough with either Rust or the existing codebase to be sure what that would be.

In addition to the v128 type and SIMD intrinsics, there's also a new i128 literal type, handled (hopefully) analogously to existing i64 literals.

I have done rudimentary spot checks of v128 constants, non-lane/non-memory SIMD operations, lane loads, lane stores, lane extractions, and lane replacements. At least one or two representative intrinsics in each of these classes seem to function correctly. That's been about the extent of my testing, however.

A few other warnings / deficiencies:

  • The lane typecheck verifies only that lane values are in the 0-15 range - it does not adapt properly to instruction lane size.
  • I didn't implement shuffle, since it would take some special handling.
  • I would be shocked if I transcribed all of the SIMD intrinsics and their typings correctly.

Formats for the new classes of instructions are:

v128.store32_lane(v128_value, lane_idx, ...memarg);
v128.load32_splat(v128_value, lane_idx, ...memarg);
i32x4.extract_lane(v128_value, lane_idx);
i32x4.replace_lane(v128_value, lane_idx, i32_value);

And here is a quick and dirty smoke test:

include "./microw8-api.cwa"

export fn upd() {
    let v = i32x4.replace_lane(i32x4.splat(255), 1, 20);
    v128.store32_lane(v, 1, USER_MEM);
    v=v128.load32_lane(v, 2, USER_MEM);
    cls(i32x4.extract_lane(v128.load32_lane(v, 2, 0, USER_MEM),2));
}

Let me know if this is something you'd potentially be interested in merging, and if so what you'd need to make that happen.

@exoticorn
Copy link
Copy Markdown
Owner

That's awesome! Generally, I'm very interested in merging this and on first glance the code looks fine so far. (Probably a few small nitpicks here and there.)

I just feel that I should read through the wasm simd spec before doing a full review and it might be a few days before I get to that.

.then_ignore(just("i64"))
.map(|n| Token::Int64(n as i64));

let int128 = integer
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

integer currently parses to an i64. So this won't be able to cover the whole 128 bit range. Maybe it's enough to update integer to parse into an i128 instead.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have worked! I also added i128 support to data sections for consistency even though I don't know why you wouldn't just use smaller units instead. I think I've managed to be consistent enough with using I128 to describe the literal type and V128 to describe the WASM type but I'm not 100% sure I got this right.

src/typecheck.rs Outdated
return type_mismatch(Some(I32), &span, param.type_, &param.span, context.sources);
}
let lane = param.const_i32();
if lane < 0 || lane > 15 {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely needs to check for the correct range for this specific instruction. The emit phase will most likely just panic when encountering an out-of-range lane.

But let me read the spec first, so that I can give a suggestion on how to best implement that.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, okay, that makes sense. It looked like there was a fairly simple way to add these checks so I've tried to do that, but I'm certainly open to alternatives!

@luchak
Copy link
Copy Markdown
Author

luchak commented Jan 24, 2023

Great! I'm glad to hear this seems to be in scope. No rush on the review - happy to pick this back up whenever you get a chance to look at the spec.

Also change the way lane widths are handled - it turns out that shuffle
takes lane arguments up to 31, so it's necessary to store maximum lane
index explicitly instead of deriving it from lane width.
@luchak
Copy link
Copy Markdown
Author

luchak commented Jan 25, 2023

Ok, I squeezed shuffle in, but that should be it, I think - I won't scope creep this any further except by request. :)

@luchak
Copy link
Copy Markdown
Author

luchak commented May 1, 2023

Process request: if you do end up looking at this and wanting me to do something, can you ping me on Discord? luchak#7452, and I think we have a couple of servers in common. My Github notifications are just a bit of a mess.

Similarly for exoticorn/microw8#4 .

If not, no worries, I'm mostly focused on work and on non-sizecoding projects for the moment, I just wanted to make sure I wouldn't be dropping the ball here.

@EmNudge
Copy link
Copy Markdown

EmNudge commented May 15, 2024

It's been a while, but I just wanted to say that I'd be really excited to see this PR land if you ever have the time to review it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants